Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix problem with low level lp_ticker STM wrapper #11385

Merged
merged 2 commits into from
Sep 5, 2019
Merged

Fix problem with low level lp_ticker STM wrapper #11385

merged 2 commits into from
Sep 5, 2019

Conversation

Tharazi97
Copy link
Contributor

Description

Fixes #11373

I have changed the STM implementation of low level lp ticker wrapper in way that it checks if event gets outdated while waiting for CMPOK flag. I have also added a solution of handling events that are close to ticker counter roll over. It changes the base of the timestamp so it can be easily compared with other values around max counter value.

/* Change the lp_delayed_counter buffor in that way so the value of (0xFFFF - LP_TIMER_SAFE_GUARD) is equal to 0.
             * By doing this it is easy to check if the value of timestamp get outdated by delaying its programming
             * For example if LP_TIMER_SAFE_GUARD is set to 5
             * (0xFFFA + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 0
             * (0xFFFF + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 5
             * (0x0000 + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 6
             * (0x0005 + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 11*/
            lp_delayed_counter = (timestamp + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF;

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@maciejbocianski @mprse @jamesbeyond @LMESTM @0xc0170

Release Notes

@Tharazi97 Tharazi97 force-pushed the lp_ticker_wrapper_change branch from 4551399 to 5cb7297 Compare August 30, 2019 14:45
@ciarmcom
Copy link
Member

@Tharazi97, thank you for your changes.
@mprse @maciejbocianski @jamesbeyond @0xc0170 @ARMmbed/mbed-os-maintainers please review.

LMESTM
LMESTM approved these changes Aug 30, 2019
View reviewed changes
Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense and is properly described ... thank you
2 minor remarks / questions

targets/TARGET_STM/lp_ticker.c Outdated
/* If this target timestamp is close to the roll over of the ticker counter
* and current tick is also close to the roll over, then we are in danger zone.*/
if(((0xFFFF - LP_TIMER_SAFE_GUARD < timestamp) || (timestamp < LP_TIMER_SAFE_GUARD))
&& ((0xFFFA < last_read_counter) || (last_read_counter < LP_TIMER_SAFE_GUARD)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the condition (last_read_counter < LP_TIMER_SAFE_GUARD) also needed ?
If this is the case, then the rollover already happened, right ?
I don't think it would cause an issue though ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. It could do some mess if you want to set timestamp to 0xFFFF, or something close to, and current tick is at the beginning of the counter.

targets/TARGET_STM/lp_ticker.c Outdated
&& ((0xFFFA < last_read_counter) || (last_read_counter < LP_TIMER_SAFE_GUARD)))
{
roll_over_flag = true;
/* Change the lp_delayed_counter buffor in that way so the value of (0xFFFF - LP_TIMER_SAFE_GUARD) is equal to 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type buffor / buffer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Tharazi97 Tharazi97 force-pushed the lp_ticker_wrapper_change branch from 5cb7297 to 36dfbca Compare August 30, 2019 17:39
@mbed-ci
Copy link

mbed-ci commented Sep 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

0xc0170
0xc0170 requested changes Sep 2, 2019
View reviewed changes
targets/TARGET_STM/lp_ticker.c Outdated
/* If this target timestamp is close to the roll over of the ticker counter
* and current tick is also close to the roll over, then we are in danger zone.*/
if(((0xFFFF - LP_TIMER_SAFE_GUARD < timestamp) || (timestamp < LP_TIMER_SAFE_GUARD)) && (0xFFFA < last_read_counter))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the style in the file (=consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Tharazi97 Tharazi97 force-pushed the lp_ticker_wrapper_change branch from 36dfbca to 3e1aa43 Compare September 2, 2019 08:40
@Tharazi97 Tharazi97 force-pushed the lp_ticker_wrapper_change branch from 3e1aa43 to a95450b Compare September 2, 2019 08:49
jamesbeyond
jamesbeyond approved these changes Sep 3, 2019
View reviewed changes
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good

@0xc0170 0xc0170 requested a review from a team September 4, 2019 12:08
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 4, 2019

Started CI

LMESTM
LMESTM approved these changes Sep 4, 2019
View reviewed changes
Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot !

@mbed-ci
Copy link

mbed-ci commented Sep 4, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM

@Tharazi97
Copy link
Contributor Author

CI fail seems not related.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Sep 5, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 merged commit b8ebee5 into ARMmbed:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesbeyond jamesbeyond jamesbeyond approved these changes

@LMESTM LMESTM LMESTM approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@maciejbocianski maciejbocianski Awaiting requested review from maciejbocianski

@mprse mprse Awaiting requested review from mprse

Assignees
No one assigned
Labels
release-version: 5.14.0-rc2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lp_ticker problem
7 participants
@Tharazi97 @ciarmcom @mbed-ci @0xc0170 @LMESTM @jamesbeyond @adbridge